Skip to content

Always Send Futures#1001

Closed
rylev wants to merge 4 commits intoAzure:mainfrom
rylev:send-all-the-things
Closed

Always Send Futures#1001
rylev wants to merge 4 commits intoAzure:mainfrom
rylev:send-all-the-things

Conversation

@rylev
Copy link
Contributor

@rylev rylev commented Aug 11, 2022 •
edited
Loading

This PR removes the distinction between wasm32 targets and other targets when it comes to whether futures are Send or not. This makes it so that futures must always be Send. This greatly simplifies the code.

In order for this to work though, we need to be able to move away from requiring Reqwest. This requires two further changes:

Thoughts?

@rylev
Copy link
Contributor Author

rylev commented Aug 18, 2022

@radu-matei @Mossaka as people working in the WASM space, what do you think of this change? This would require all implementations to produce Send futures. This means that if you're making network calls (for example, in a wasi environment), your HTTP implementation must be fine being sent to another thread.

This also means that this crate is not really useable for browser based WebAssembly (since the http implementation is !Send) and probably even for node.js. There are work arounds like send_wrapper which makes sending a !Send value possible (and using it from another thread a runtime panic), but we probably don't want to require users to do this.

@rylev rylev force-pushed the send-all-the-things branch from 353d418 to 081eb94 Compare August 18, 2022 09:14
@ctaggart
Copy link
Contributor

ctaggart commented Aug 23, 2022

This also means that this crate is not really useable for browser based WebAssembly

Our crates need to be usable in the browser via WebAssembly.

@rylev
Copy link
Contributor Author

rylev commented Aug 23, 2022

@ctaggart can you talk more through the use case for browser WASM? Wouldn't there be issues with cors?

@rylev
Copy link
Contributor Author

rylev commented Aug 23, 2022

Discussed this with folks, and we agree we want to support running this SDK in the browser.

@rylev rylev closed this Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danieljurek danieljurek Awaiting requested review from danieljurek danieljurek will be requested when the pull request is marked ready for review danieljurek is a code owner

@weshaggard weshaggard Awaiting requested review from weshaggard weshaggard will be requested when the pull request is marked ready for review weshaggard is a code owner

@heaths heaths Awaiting requested review from heaths heaths will be requested when the pull request is marked ready for review heaths is a code owner

@RickWinter RickWinter Awaiting requested review from RickWinter RickWinter will be requested when the pull request is marked ready for review RickWinter is a code owner

@LarryOsterman LarryOsterman Awaiting requested review from LarryOsterman LarryOsterman will be requested when the pull request is marked ready for review LarryOsterman is a code owner

@vincenttran-msft vincenttran-msft Awaiting requested review from vincenttran-msft vincenttran-msft will be requested when the pull request is marked ready for review vincenttran-msft is a code owner

@jalauzon-msft jalauzon-msft Awaiting requested review from jalauzon-msft jalauzon-msft will be requested when the pull request is marked ready for review jalauzon-msft is a code owner

@chlowell chlowell Awaiting requested review from chlowell chlowell will be requested when the pull request is marked ready for review chlowell is a code owner

@ronniegeraghty ronniegeraghty Awaiting requested review from ronniegeraghty ronniegeraghty will be requested when the pull request is marked ready for review ronniegeraghty is a code owner

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rylev @ctaggart